Consider wrapping the enum in a struct to emphasize that the user should not try to build the variants directly, but instead use the methods.

Given the structs WithPeerId, WithPeerIdWithAddresses and WithoutPeerIdWithAddress can not be constructed outside of dial_opts, the user can not build DialOpts themselves. Still I understand that this is not intuitive and that it would be nice to make it clear that the user should go through the methods on DialOpts.

Is wrapping an enum into a struct an idiomatic way in Rust to signal that something should not be constructed directly? I don't recall seeing this in the past. Any prominent examples? I would have just relied on a doc comment on DialOpts.

Is wrapping an enum into a struct an idiomatic way in Rust to signal that something should not be constructed directly? I don't recall seeing this in the past.

I would argue it the other way around: I feel like it is very rare and not rust-idiomatic that an enum is not constructed directly. Of course a user would notice that it is not possible once they try to build the inner structs, but it still may cause some confusion. With a struct, the API would make it clearer that it should be constructed via methods, especially when looking at it on docs.rs.

I don't recall seeing this in the past. Any prominent examples?

I feel like this is just following the general api design pattern of wrapping internal/ inner types in a new struct to hide implementation details. Imo this always makes sense when the inner type is not relevant for the user and should never be constructed directly by them, which is the case here.

Alternatively, if you decide to fold the structs into the enum variants as you wrote in your TODO in l.1786:

// TODO: How about folding these structs into the enum variants

the user would be able to build the enum types directly and the builder pattern would not be needed at all.

Addressed in ad083e9.

Maybe add this method here as well.

I am a bit reluctant to offer two ways to achieve the same thing. In what use-case would one want to call condition on WithPeerIdWithAddresses and couldn't do so on WithPeerId?

I am also a bit undecided in this aspect.
But the DialOpts are trying to follow the syntax of a builder pattern, and with a builder it normally doesn't matter in which order you specify the paramters, so you would expect that DialOpts::peer_id(...).condition(..).addresses(..) and DialOpts::peer_id(...).addresses(..).condition(..) both work.
That being said, the current implementation doesn't strictly follow the builder pattern anyway (at least not the way it is described in the Rust API Guidelines), therefore I also see the point of avoiding duplicated code.

Addressed in 26dedbe.

I do like the consistent syntax of DialOpts::peer_id(..).addresses() and DialOpts::unknown_peer_id(..).address(..).
But this similar syntax suggests that WithPeerId and WithoutPeerId are alike, which is not the case because WithPeerId can be a "final" state from which you can call build(), whereas WithoutPeerId is only intermediate. Not sure about this, but I feel like it could end up confusing the user, since even though it follows the same naming convention, WithoutPeerId is not a valid DialOpt.

Optionally if we follow the above suggestion, we could keep it consist by also adding

impl WithoutPeerIdWithAddress {
        pub fn peer_id(self, peer_id: PeerId) -> WithPeerIdWithAddresses {
            WithPeerIdWithAddresses {
                peer_id,
                condition: Default::default(),,
                addresses: vec![self.address],
                extend_addresses_through_behaviour: false,
            }
        }
    }

The only problem with this is that without a peer id you only have one address, whereas with an peer id there can be multiple.

Say we allowed DialOpts::address(..) instead of DialOpts::unknown_peer_id(..).address(..), would that not confuse a user into thinking that they can then later on add a peer id, i.e. that they can do DialOpts::address(..).peer_id(..)?

The only problem with this is that without a peer id you only have one address, whereas with an peer id there can be multiple.

Yes, this is something that I would like to encode at the type level, thus the differentiation between the two. I am not sure how to model your suggestion without loosing the type safety.

Say we allowed DialOpts::address(..) instead of DialOpts::unknown_peer_id(..).address(..), would that not confuse a user into thinking that they can then later on add a peer id, i.e. that they can do DialOpts::address(..).peer_id(..)?

I agree. But with reference to the above discussion: the pattern currently already expects a fixed order in which the parameters are specified, e.g. as written above: in case of a known peer id you have to specify the condition before specifying the addresses. Isn't that kind of the same issue?

Yes, this is something that I would like to encode at the type level, thus the differentiation between the two. I am not sure how to model your suggestion without loosing the type safety.

You are right, the type safety should be kept; DialOpts::address(..).peer_id(..) should not be possible.

Given how common the following is:

DialOpts::unknown_peer_id().address(listen_addr).build()

We could consider accepting impl Into<DialOpts>:

and implement:

impl From<MultiAddress> for DialOpts { }

where we do exactly the above.

Totally agree we can get rid of a boilerplate code like this. Another option is to accept an impl DialOptsBuilder and call: .dial(DialOpts::.address(listen_addr)).

Given how common the following is:

DialOpts::unknown_peer_id().address(listen_addr).build()

We could consider accepting impl Into<DialOpts>:

and implement:

impl From<MultiAddress> for DialOpts { }

where we do exactly the above.

Neat! Thanks for bringing it up @thomaseizinger. I will give this a shot.

5e3edb3 implements From<Multiaddr> and changes calls to unknown_peer_id().address(xxx).build() to the address only.

Totally agree we can get rid of a boilerplate code like this. Another option is to accept an impl DialOptsBuilder and call: .dial(DialOpts::.address(listen_addr)).

I think From<Multiaddr> is the more elegant way. Given that the two solve the same issue, I would prefer the earlier over the later suggestion.

This API requires users to import one more type: PeerCondition. Might be worth providing 3 functions instead that configure the corresponding condition.

Please excuse if that has been discussed and dismissed already!

Modelling 3 exclusive states with 3 methods instead of an enum seems not intuitive to me. Is the additional import of PeerCondition worth the added complexity?

I think there is a misunderstanding. I would still model the states internally with an enum. However, nothing stops anyone from doing this:

DialOpts::address(addr).condition(...).condition(...)

It might give people a hint that the condition is overridden but in theory, it could also be implemented in an additive way.
I don't feel strongly about it so happy to keep the enum :)

I think there is a misunderstanding. I would still model the states internally with an enum.

No misunderstanding :)

I don't feel strongly about it so happy to keep the enum :)

Will keep the enum for now. Not 100% happy but can't think of a better alternative. We can still further iterate on it. Merging for now.

impl From<PeerId> for DialOpts too?

👍 good idea. Done via 09293c4.